Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix errors in Restore_Reference_Defaults() subroutine #427

Merged

Conversation

illorenzo7
Copy link
Contributor

@illorenzo7 illorenzo7 commented Dec 24, 2022

The Restore_Reference_Defaults() routine of PDE_Coefficients was a bit out of date. Some variables were not restored to their proper values, or at all. Not all the allocatable arrays were deallocated. This pull request should fix these errors. It incorporates the code cleanup from my other pull request (#426 ).

@illorenzo7
Copy link
Contributor Author

Hmmm....these pull requests are now failing after merging @feathern's custom reference tests. Fortunately, this one (which only modifies the "Restore_Reference_Defaults()" routine) is also failing, which means there must be a problem with my fix to the restore defaults---probably specifically related to the custom reference stuff. Let me look into this. Hopefully it's just an issue with "Restore_Reference_Defaults()" and an easy fix.

@feathern
Copy link
Contributor

feathern commented Jan 4, 2023

Looks like your fix passed the check. I'm cooking dinner right now, but I'll review this in the morning and we can go from there. And yes, the restore_reference_defaults was way out of date. A conversation for our monthly meeting I think (others are out of date too).

@illorenzo7
Copy link
Contributor Author

Yes, it seems like I can't reference the custom reference stuff in the restore defaults routine yet. I'm not entirely sure what's going on, or whether the problem is on this end, or the new custom tests' end. Anyway, I will try rebasing the other pull requests of this (slightly "incomplete" Restore_Reference_Defaults() routine--should be a relatively easy thing to fix later, and I'd like to get that other stuff (which feels more critical, since there were some bugs) rolling. Hopefully this was the whole issue, and the other pull requests pass the checks after rebasing. We'll see!

@feathern
Copy link
Contributor

feathern commented Jan 5, 2023

Two things:

  1. I can merge this once the commented lines are deleted rather than commented. I'm referring to the lines you had to fix to make this work.
  2. The issue here was that restore_reference_defaults is called as part of the benchmarking initialization. We wanted to ensure that benchmarking mode could be used as a way of testing the custom reference state framework. When benchmarking mode is used alongside a custom reference state, a few variables (like ref_type) are saved and restored following the call to restore_reference_defaults. The lines you commented out were effectively undoing whatever had previously been read in from main_input describing the custom reference state, and so the tests were rightfully failing.

It's worth noting that restore_reference_defaults is now only used for benchmarking and doesn't necessarily need to actually restore all of the default values as it did in the past.

@illorenzo7
Copy link
Contributor Author

OK, I will go ahead and delete these. I'm fairly unfamiliar with these routines. Basically, I went off the lines

1145 ! Restore all variables in this module to their default state.
1146 ! Deallocates all allocatable module variables.

Anyway, it sounds like we'll be discussing these routines in general, so as long as you think it's doing what it's supposed to now, that's fine.

...in Restore_Reference_Defaults(). As per Nick's request.
@illorenzo7
Copy link
Contributor Author

Just fixed this but it triggered another test (I submitted commit directly from Github).

Copy link
Contributor

@feathern feathern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, looks good. Merging now.

@feathern
Copy link
Contributor

feathern commented Jan 5, 2023

I went ahead and resolved the merge conflict.

@feathern feathern merged commit 9c03c16 into geodynamics:main Jan 5, 2023
@illorenzo7 illorenzo7 deleted the fix_restore_reference_defaults branch February 14, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants